-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370
base: 4.1.x
Are you sure you want to change the base?
Conversation
Just pushed a change to fix the psalm issues |
It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those. |
ಠ_ಠ |
Codecov says that some of the new code is untested, most notably the parts using You can learn more about such tests at https://www.doctrine-project.org/projects/doctrine-dbal/en/stable/reference/testing.html |
I think I found the reason: Let me retry this. |
In a recent PR, we got reports from CodeCov about several lines not being covered. After further inspection, I found an error message in the coverage file upload saying we hit a rate limit. This will make it more likely we spot such issues in the future. The PR: doctrine#6370
From the Codecov UI:
… we currently send 69 uploads, so there is no way we are going to be below the upload limit. I will have to look into this separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, my feedback is not a blocker.
public function normalizeCollation(string $collation): string | ||
{ | ||
if ($this->useUtf8mb3 && str_starts_with($collation, 'utf8_')) { | ||
return 'utf8mb3' . substr($collation, 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make things more obvious here:
return 'utf8mb3' . substr($collation, 4); | |
return 'utf8mb3' . substr($collation, strlen('utf8')); |
} | ||
|
||
if (! $this->useUtf8mb3 && str_starts_with($collation, 'utf8mb3_')) { | ||
return 'utf8' . substr($collation, 7); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return 'utf8' . substr($collation, 7); | |
return 'utf8' . substr($collation, strlen('utf8mb3')); |
Ah there is one issue though: since this is a fix, you should target 3.8.x |
I can create another PR for this branch. |
Please don't, we usually do merges up. Instead, let me retarget your PR, and please rebase your commits on 3.8.x and force push. |
In a recent PR, we got reports from CodeCov about several lines not being covered. After further inspection, I found an error message in the coverage file upload saying we hit a rate limit. This will make it more likely we spot such issues in the future. The PR: doctrine#6370
The PR uses I'm going to be offline for the next week or so, and will look at this on my return. |
Enjoy your time off 👍 |
@fisharebest do you need help with the rebase? |
I have rewritten my patch to use the However, I have been unable to back-port this PR to the 3.8.x branch. (I can, but it doesn't fix the bug...). The handling of charset/collation is different. I'd probably need to copy lots of other changes from the 4.0.x branch to make it work. Options are:
|
I'd be surprised if there were lots of changes you could backport that are not breaking changes, so I'd say we should consider applying this to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All seems ok
As far as I understand this, utf8 would stay utf8 once utf8mb3 isn't supported anymore. I didn't find information about when this alias handling was introduced on a short search though. |
Now that MySQL 9.0 is released, we can see that the logic still works, and the tests pass. |
Can you please rebase onto 4.1? That should trigger a CI pipeline that includes MySQL 9. |
Summary
In MySQL (and MariaDB),
utf8
andutf8mb3
are aliases. Depending on the database server version, the information schema may contain one or the other. Similarly, column definitions may contain one or the other.When performing schema-comparisons, a mismatch between
utf8
andutf8mb3
causes anALTER TABLE
statement to be generated - even when columns are identical.The code already contains a function to normalize the charset/collation options:
dbal/src/Platforms/MySQL/Comparator.php
Line 83 in 61d79c6
My solution is to extend this function so that it also replaces
utf8
withutf8mb3
(or vice-versa), to match the current database connection.To detect the prefered character set for the connection, I've added:
AbstractMySQLPlatform::informationSchemaUsesUtf8mb3()
MariaDBPlatform::informationSchemaUsesUtf8mb3()
The obvious place for the new normalization logic is the
CharsetMetadataProvider
andCollationMetadataProvider
classes. These are interfaces, so this is technically a BC break, however, they are marked@internal
.I've added tests which should cover all the new/modified code. I've run it against MySQL. I don't have MariaDB to hand.